@claude review

Claude finished @PauliusInvertus's task in 2m 2s —— View job

Code Review — BRD-804 Synonym Configuration

• Gather context and understand the request
• Read changed files and diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

The PR cleanly achieves its goal: synonym groups are serialized to/from Solr equivalence strings ("laptop, notebook, computer") at the API boundary while the SDK surface stays in native PHP arrays. The layering is sensible and the test coverage is solid. A few issues worth addressing before merge:

Issues

1. SynonymConfiguration::fromApiResponse() throws on empty synonyms list

fromApiResponse() feeds the parsed synonyms directly into new self(...), which calls validateSynonyms(). If the API ever returns "synonyms": [], construction throws InvalidArgumentException: Synonyms array cannot be empty. — a valid API state treated as a programming error.

src/V2/ValueObjects/Synonym/SynonymConfiguration.php lines 66–74

public static function fromApiResponse(array $data): self
{
    $synonyms = array_map(
        static fn(string $group): array => array_map('trim', explode(',', $group)),
        $data['synonyms'] ?? []
    );

    return new self((string) ($data['language'] ?? ''), $synonyms);
    //                                                  ^^^^^^^^ throws if empty
}

The same edge case applies when a consumer calls withSynonyms([]) — but there the exception is intentional (you're constructing a config to send). Here you're deserialising an incoming response, so the class should handle it gracefully (perhaps returning null or wrapping with fromApiResponse returning a nullable).

Fix this →

2. fromApiResponse() is less defensive than normalizeSynonyms()

SynonymResponse::normalizeSynonyms() correctly handles both Solr strings and already-normalised arrays:

static fn(string|array $group): array => is_string($group)
    ? array_map('trim', explode(',', $group))
    : $group

SynonymConfiguration::fromApiResponse() does not — it assumes all entries are strings:

static fn(string $group): array => array_map('trim', explode(',', $group))

If a caller passes already-normalised arrays, PHP will trigger a TypeError on the string param hint. Given $data is typed array<string, mixed>, this is reachable without a static analysis warning. The two methods should share the same defensive pattern.

Fix this →

3. getSynonyms() inline comment is misleading

src/SyncV2Sdk.php line 219

// GET returns {language, synonyms} without count/reindex fields.

The comment says the GET response never has synonym_count/requires_reindex, but the code immediately after uses ?? to fall back gracefully when they are present. The comment overstates the certainty and could mislead a future reader who sees a GET response that does include those fields.

Fix this →

4. testMatchesOpenApiExampleResponse in SynonymResponseTest doesn't test the Solr string path

The test is named testMatchesOpenApiExampleResponse and the fixture (synonyms-ecommerce-en.json) was updated to Solr strings — but the test still passes nested PHP arrays as the simulated API response. The Solr-string parsing is covered by testFromArrayNormalizesSolrStringSynonymsIntoGroups, so this isn't a correctness gap, but the inconsistency between the test name and its data is a readability concern.

Minor Observations

• fromApiResponse() is not called anywhere in SyncV2Sdk — it appears to be a utility for SDK consumers who hold a raw GET response. Its intended use case could benefit from a short usage example in the docblock, or it could be removed until there's a concrete caller.

• Single-term synonym group (["laptop"]) passes validation — which may be intentional (a synonym group of 1 is a no-op for Solr but may be semantically meaningful elsewhere). Worth a comment if it's deliberate.

• The Solr => detection (str_contains($term, '=>')) correctly catches explicit one-way mappings. Solid guard.

Positives

• Clean separation of internal (PHP arrays) vs. wire (Solr strings) representations.
• normalizeSynonyms() in SynonymResponse handles both string and array forms defensively — good forward compatibility.
• getSynonyms() null-coalescing for missing response fields is robust.
• The fixture update and the testJsonEncodeProducesValidJson round-trip test are good sanity checks.
• Validation of Solr syntax characters (,, =>) guards against malformed payloads at construction time, not at serialization time.

@claude please look at code one more time and give a feedback on declarative programming implementation for readability purposes.

Claude finished @PauliusInvertus's task in 1m 49s —— View job

Declarative Programming Review — BRD-804 Synonym Configuration

• Gather context and understand the request
• Read changed files
• Analyze declarative style and readability
• Post feedback

The PR has solid declarative bones — named constructors, immutable value objects, array_map pipelines. But there are a few areas where the imperative style leaks through and reduces readability. Below are the observations ranked by impact.

1. validateSynonyms() — biggest readability win available

src/V2/ValueObjects/Synonym/SynonymConfiguration.php lines 119–190 is a 70-line method with two nested foreach loops and five separate if/throw blocks. The reader has to mentally track two loop indices simultaneously to understand what is being validated. Extracting into two focused methods tells the story in a single glance:

private function validateSynonyms(array $synonyms): void
{
    if (empty($synonyms)) {
        throw new InvalidArgumentException('Synonyms array cannot be empty.', 'synonyms', $synonyms);
    }

    foreach ($synonyms as $index => $group) {
        $this->validateSynonymGroup($index, $group, $synonyms);
    }
}

private function validateSynonymGroup(int $index, mixed $group, array $synonyms): void
{
    if (!is_array($group)) {
        throw new InvalidArgumentException(
            sprintf('Synonym group at index %d must be an array, got %s.', $index, gettype($group)),
            'synonyms', $synonyms
        );
    }
    if (empty($group)) {
        throw new InvalidArgumentException(
            sprintf('Synonym group at index %d cannot be empty.', $index),
            'synonyms', $synonyms
        );
    }

    foreach ($group as $termIndex => $term) {
        $this->validateSynonymTerm($index, $termIndex, $term, $synonyms);
    }
}

private function validateSynonymTerm(int $index, int $termIndex, mixed $term, array $synonyms): void
{
    if (!is_string($term)) { /* ... */ }
    if (trim($term) === '') { /* ... */ }
    if (str_contains($term, ',') || str_contains($term, '=>')) { /* ... */ }
}

Each method now declares one concern. The single-responsibility rule reads more naturally than scanning 70 lines for the shape of the validation logic.

Fix this →

2. Solr-string parsing logic is split across two classes without a shared name

SynonymConfiguration::fromApiResponse() and SynonymResponse::normalizeSynonyms() both implement the same "Solr string → array of trimmed terms" transform but with slight differences (one uses a string hint, the other string|array). Declarative code names the operation:

// A shared private static helper, or a utility class:
private static function parseSolrGroup(string|array $group): array
{
    return is_string($group)
        ? array_map('trim', explode(',', $group))
        : $group;
}

Now both call sites become self-documenting:

// fromApiResponse():
$synonyms = array_map(static fn($g) => self::parseSolrGroup($g), $data['synonyms'] ?? []);

// normalizeSynonyms():
return array_map(static fn($g) => self::parseSolrGroup($g), $synonyms);

The reader no longer needs to parse the is_string ternary to understand what is happening — parseSolrGroup announces the intent.

Fix this →

3. getSynonyms() — imperative normalization buried inline

SyncV2Sdk.php lines 220–227 reads as an imperative workaround: extract a field, compute a fallback count, reassemble a fresh array. The intent — "the GET endpoint omits metadata fields; provide defaults" — is lost in the mechanics:

$synonyms = $response['synonyms'] ?? [];

return SynonymResponse::fromArray([
    'language'         => $response['language'] ?? $language,
    'synonym_count'    => $response['synonym_count'] ?? count($synonyms),
    'requires_reindex' => $response['requires_reindex'] ?? false,
    'synonyms'         => $synonyms,
]);

Extracting into a named method declares what is happening:

public function getSynonyms(string $language): SynonymResponse
{
    $response = $this->getHttpClient()->get(
        $this->baseApiPath . 'synonyms?language=' . urlencode($language)
    );

    return SynonymResponse::fromArray(
        $this->withSynonymGetDefaults($response, $language)
    );
}

private function withSynonymGetDefaults(array $response, string $language): array
{
    $synonyms = $response['synonyms'] ?? [];

    return [
        'language'         => $response['language'] ?? $language,
        'synonym_count'    => $response['synonym_count'] ?? count($synonyms),
        'requires_reindex' => $response['requires_reindex'] ?? false,
        'synonyms'         => $synonyms,
    ];
}

The public method now reads as a clean pipe: fetch → apply defaults → parse.

4. testAcceptsManySynonymGroups() — loop in test data setup

tests/V2/ValueObjects/Synonym/SynonymConfigurationTest.php lines 400–409 builds test data with a for loop:

$synonyms = [];
for ($i = 0; $i < 100; $i++) {
    $synonyms[] = ["term{$i}a", "term{$i}b"];
}

The declarative equivalent removes the mutation entirely:

$synonyms = array_map(
    fn(int $i): array => ["term{$i}a", "term{$i}b"],
    range(0, 99)
);

range + array_map expresses "a sequence of N pairs" without a mutable accumulator.

What is already working well

• jsonSerialize() in both classes — pure array_map pipelines with no intermediate variables.
• The with*() / addSynonym() builder pattern — canonical immutable/declarative PHP.
• Named constructors (fromArray, fromApiResponse) — callers declare what they have, not how to construct.
• normalizeSynonyms() ternary in SynonymResponse — expresses the conditional cleanly.
• array_key_exists inside validateRequiredFields() is explicit and unambiguous — no change needed there.

Correctness:

• Guard getSynonyms against a non-array synonyms response before count()
• fromApiResponse() returns null for an empty synonym list instead of
  throwing, since an empty list is a valid incoming API state
• Reword getSynonyms comment to reflect the ?? field defaulting

Readability:

• Split validateSynonyms() into validateSynonymGroup()/validateSynonymTerm()
• Extract getSynonyms normalization into withSynonymGetDefaults()
• Share Solr-string parsing via NormalizesSynonymGroups trait
• Build test data with range()+array_map() instead of a for loop
• testMatchesOpenApiExampleResponse now loads the Solr-string fixture

@claude review

Claude finished @PauliusInvertus's task in 2m 59s —— View job

Code Review — BRD-804 Synonym Configuration (Post-Feedback Update)

• Gather context and understand the request
• Read changed files and diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

The PR is in very good shape after incorporating the earlier review feedback. All major points were addressed cleanly:

• validateSynonyms() split into validateSynonymGroup() + validateSynonymTerm() ✅
• getSynonyms() normalization extracted into withSynonymGetDefaults() ✅
• Solr-string parsing consolidated in the NormalizesSynonymGroups trait ✅
• fromApiResponse() returns ?self for empty synonym lists ✅
• Test data built with range() + array_map() ✅
• testMatchesOpenApiExampleResponse now loads the Solr-string fixture ✅
• Non-array $synonyms guard added in withSynonymGetDefaults() ✅

Two issues remain worth addressing before merge.

Issues

1. SynonymResponse::normalizeSynonyms() can TypeError on a non-null non-array value

normalizeSynonyms(?array $synonyms) has a strict ?array type hint. fromArray() calls it as:

synonyms: self::normalizeSynonyms($data['synonyms'] ?? null)

With declare(strict_types=1), if $data['synonyms'] is any non-null non-array value (e.g. a string "laptop, notebook" — the whole synonyms field as a single string), PHP throws a TypeError before the method body is reached. withSynonymGetDefaults() guards against this for the getSynonyms() path, but fromArray() is public and callable directly by SDK consumers.

The fix is a one-liner guard inside normalizeSynonyms:

private static function normalizeSynonyms(mixed $synonyms): ?array
{
    if (!is_array($synonyms)) {
        return null;
    }
    return array_map(self::normalizeGroup(...), $synonyms);
}

Fix this →

2. normalizeGroup returning [] is treated differently by its two consumers

The trait documents that non-string non-array values "collapse to an empty group". The two call sites handle this silently-empty result differently:

• SynonymConfiguration::fromApiResponse(): passes the result through new self(), which calls validateSynonymGroup() and throws InvalidArgumentException for the empty group. ✅ Correct — construction fails loudly.
• SynonymResponse::normalizeSynonyms(): passes the result straight into the returned array with no validation. A malformed API entry silently produces [] inside synonyms. ⚠️ Inconsistent — the same input produces a silent empty group here.

For SynonymResponse, this is less critical (it's a read-only deserialization object), but the inconsistency could confuse a future reader who sees [] inside a synonyms array and cannot trace where the empty group came from. Either document this divergence in the trait docblock, or filter out empty groups in normalizeSynonyms():

return array_filter(
    array_map(self::normalizeGroup(...), $synonyms),
    fn(array $group): bool => !empty($group)
);

Minor Observations

• testMatchesOpenApiExampleResponse fixture merge: $fixture + ['synonym_count' => ..., 'requires_reindex' => ...] uses the + operator which does not overwrite existing keys. If the fixture ever gains those keys the test silently defers to fixture values rather than the computed ones. Using array_merge() would make the intent explicit and the test more resilient.

• PHPDoc mismatch in normalizeSynonyms: The @param says array<int, mixed>|null but the signature says ?array. With PHP 8 generics (even in PHPDoc form), this should be array<int, mixed>|null on the @param while the actual type hint remains ?array (or mixed as proposed above). Minor cleanup.

What Landed Well

• NormalizesSynonymGroups trait is the right shape — a focused, reusable single method that both classes use without inheritance coupling.
• withSynonymGetDefaults() makes getSynonyms() a clean 3-line pipe: fetch → defaults → parse.
• The is_array($synonyms) ? $synonyms : [] double guard in withSynonymGetDefaults() addresses the Gemini-flagged null-synonyms crash.
• fromApiResponse() returning ?self is semantically precise: no synonyms from the API is not the same as a programmer passing an invalid argument.
• New tests testGetSynonymsHandlesEmptyResult and testFromApiResponseReturnsNullForEmptySynonyms cover exactly the edge cases the prior review flagged.
• testMatchesOpenApiExampleResponse now exercises the actual Solr-string fixture, making it a real integration-style sanity check.

@claude review

Claude finished @PauliusInvertus's task in 3m 20s —— View job

Code Review — BRD-804 Synonym Configuration (Third Pass)

• Gather context and understand the request
• Read changed files and current diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

Both previously flagged issues are resolved. The implementation is clean and the PR is close to merge-ready. Three minor observations remain — none are blockers, but two of them could surprise a future contributor.

Resolved From Prior Review ✅

• normalizeSynonyms now accepts mixed and returns null on non-array input — no more TypeError risk ✅
• Malformed groups that normalize to [] are now filtered by normalizeSynonyms — no silent empty groups ✅

Remaining Observations

1. normalizeGroup passes array inputs through without trimming or type-checking elements

src/V2/ValueObjects/Synonym/NormalizesSynonymGroups.php line 27

return is_array($group) ? array_values($group) : [];

When the group arrives as a pre-normalized array, elements pass through as-is. This has two consequences:

• Trailing whitespace is preserved — ['laptop ', ' notebook'] stays ['laptop ', ' notebook']. Solr-string groups get trim() applied element-by-element; array groups do not. If the API ever returns pre-normalized arrays with whitespace, the term would serialize back with the spaces: "laptop , notebook".
• Non-string elements silently propagate into SynonymResponse::synonyms — ['laptop', 123] passes through. The PHPDoc says array<int, string> but this isn't enforced. SynonymConfiguration::fromApiResponse catches this downstream (in validateSynonymTerm), but SynonymResponse::normalizeSynonyms does not.

The Solr-string path is today's primary path, so these are low-risk. But they're a trap for future contributors who add an array-format API variant. Trimming elements in the array branch and/or adding a is_string($element) guard in normalizeGroup closes both gaps:

if (is_string($group)) {
    return array_map('trim', explode(',', $group));
}

if (is_array($group)) {
    return array_values(array_filter(
        array_map(static fn($e): string => is_string($e) ? trim($e) : '', $group),
        static fn(string $e): bool => $e !== ''
    ));
}

return [];

Fix this →

2. fromApiResponse throws on malformed entries; normalizeSynonyms silently drops them — undocumented gap

normalizeSynonyms documents its drop-on-malformed behavior clearly. fromApiResponse does not document the symmetric but opposite behavior: if the API returns e.g. ['laptop, notebook', null], normalizeSynonyms produces [['laptop', 'notebook']], while fromApiResponse throws InvalidArgumentException (because normalizeGroup(null) → [] → validateSynonymGroup rejects the empty group).

This means calling fromApiResponse on the same raw payload that SynonymResponse::fromArray handled gracefully can throw. Worth a one-liner in the fromApiResponse docblock:

 * @throws InvalidArgumentException If a synonym group cannot be parsed
 *                                   (e.g. a non-string/non-array API entry),
 *                                   unlike SynonymResponse::normalizeSynonyms()
 *                                   which drops such groups silently.

Fix this →

3. getSynonyms() always returns synonyms: [] for empty; fromArray() returns synonyms: null

withSynonymGetDefaults coerces null and non-array synonym payloads to [], so getSynonyms() always sets $response->synonyms to an array (even if empty). But calling SynonymResponse::fromArray(...) directly without a synonyms key calls normalizeSynonyms(null) → returns null.

An SDK consumer who writes:

if ($response->synonyms !== null) { /* has synonyms */ }

gets different results depending on whether they went through getSynonyms() or called fromArray() manually. This is minor and the behavior is internally consistent, but a note in the getSynonyms docblock clarifying that it always returns a non-null synonyms array would prevent confusion.

What Is Working Well

• NormalizesSynonymGroups trait is focused and single-purpose — both consumers compose it cleanly.
• withSynonymGetDefaults makes getSynonyms a clean pipe: fetch → normalise → parse. Easy to read.
• The defensive is_array($synonyms) ? $synonyms : [] guard in withSynonymGetDefaults covers the non-array API response case.
• fromApiResponse returning ?self is semantically precise: empty synonyms from the API ≠ programmer error.
• normalizeSynonyms(mixed) with !is_array → null is the correct defensive posture for a public entry point.
• array_values(array_filter(...)) in normalizeSynonyms means $response->synonyms is always densely keyed — no risk of gaps from dropped entries.
• Test coverage is thorough: testFromArrayHandlesNonArraySynonymsWithoutTypeError, testFromArrayDropsMalformedGroupsThatNormalizeToEmpty, testGetSynonymsHandlesEmptyResult, testFromApiResponseReturnsNullForEmptySynonyms all exercise the exact edge cases that were raised in prior reviews.
• testMatchesOpenApiExampleResponse now exercises the actual Solr-string fixture, making it a genuine integration-style sanity check.

@claude review

Claude finished @PauliusInvertus's task in 2m 38s —— View job

Code Review — BRD-804 Synonym Configuration (Fourth Pass)

• Gather context and understand the request
• Read changed files and current diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

All three issues from the third-pass review are resolved in the latest commit (5332eb9). The implementation is clean, robust, and ready to merge. One low-priority observation remains.

Resolved From Prior Review ✅

• normalizeGroup now trims array-form string elements and discards non-string elements ✅
• fromApiResponse docblock documents the throw-vs-drop divergence with SynonymResponse::normalizeSynonyms() ✅
• getSynonyms docblock clarifies the always-non-null synonyms guarantee ✅

One Remaining Minor Observation

String branch of normalizeGroup doesn't filter empty terms; array branch does

src/V2/ValueObjects/Synonym/NormalizesSynonymGroups.php lines 24–25

The array branch explicitly discards empty strings after trimming. The string branch does not:

// String branch — no empty-term filter
return array_map('trim', explode(',', $group));

// Array branch — empty terms discarded
return array_values(array_filter(
    array_map(static fn(mixed $term): string => is_string($term) ? trim($term) : '', $group),
    static fn(string $term): bool => $term !== ''
));

Consequence: a malformed Solr string like "," produces ['', ''], while the equivalent array ['', ''] is normalized to [] by the array branch. In fromApiResponse, the empty terms hit validateSynonymTerm and throw correctly. In SynonymResponse::normalizeSynonyms, the !empty($group) filter keeps [''] because it's a non-empty array — so a malformed API response would silently land in $response->synonyms with empty-string terms.

This requires the API to send a genuinely malformed Solr string (e.g. "," or ""), so it is low-risk in practice. A one-liner addition aligns both branches:

if (is_string($group)) {
    return array_values(array_filter(
        array_map('trim', explode(',', $group)),
        static fn(string $term): bool => $term !== ''
    ));
}

Not a blocker — the API should never send such input — but worth a follow-up if you want parity between the two branches.

Fix this →

What Is Working Well

• NormalizesSynonymGroups trait is focused, composable, and both consumers use it correctly.
• withSynonymGetDefaults makes getSynonyms a readable pipe: fetch → defaults → parse.
• The is_array($synonyms) ? $synonyms : [] guard in withSynonymGetDefaults is correct.
• normalizeSynonyms(mixed) with !is_array → null is the right defensive posture for a public entry point.
• array_values(array_filter(...)) in normalizeSynonyms ensures no sparse/empty groups pass through.
• fromApiResponse returning ?self is semantically precise and well-documented.
• validateSynonyms split into validateSynonymGroup + validateSynonymTerm reads clearly.
• Test coverage is thorough: testFromArrayTrimsArrayFormGroupsAndDropsNonStringElements, testFromArrayDropsMalformedGroupsThatNormalizeToEmpty, testGetSynonymsHandlesEmptyResult, testFromApiResponseReturnsNullForEmptySynonyms, and the range()+array_map() test-data rewrite all landed cleanly.
• testMatchesOpenApiExampleResponse now exercises the real Solr-string fixture with array_merge (correct — overwrites fixture defaults rather than deferring to them).

This PR is ready to merge. The string-branch empty-term gap is low-priority and can be a follow-up if parity is desired.

@claude review

Claude finished @PauliusInvertus's task in 2m 35s —— View job

Code Review — BRD-804 Synonym Configuration (Fifth Pass)

• Gather context and understand the request
• Read changed files and current diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

The last remaining observation from the fourth pass — the Solr-string branch of normalizeGroup not filtering empty terms — is fully resolved by commit 163ed46. The fix extracted a shared trimTerms() helper that both branches now call, giving them identical trim-and-filter behavior. This is the right approach: one code path, no parity drift.

This PR is ready to merge. No issues remain.

Resolved From Prior Review ✅

• trimTerms() extracted as a shared private static helper in NormalizesSynonymGroups ✅
• Both the string branch (explode(',', ...)) and array branch now call trimTerms() ✅
• testFromArrayDropsEmptyTermsFromSolrStringGroups covers the Solr-string empty-term case directly ✅

Final Code Walkthrough

The trait shape is clean:

// normalizeGroup: routes to the right parse strategy, then trims
if (is_string($group)) → trimTerms(explode(',', $group))
if (is_array($group))  → trimTerms(map(fn(mixed $t) => is_string($t) ? $t : '', $group))
else                   → []

// trimTerms: shared filter, one implementation
return array_values(array_filter(array_map('trim', $terms), fn($t) => $t !== ''))

Both branches go through trimTerms, so "a,,b" → ['a', 'b'] and ['a', '', 'b'] → ['a', 'b'] consistently.

The rest of the stack is coherent:

• SynonymResponse::normalizeSynonyms(mixed) accepts any input, maps through normalizeGroup, then drops empty groups — silent on malformed input, never throws.
• SynonymConfiguration::fromApiResponse(?self) maps through normalizeGroup, then routes to the constructor which validates — throws on malformed input, documented in the docblock.
• SyncV2Sdk::withSynonymGetDefaults guards the null/non-array synonyms edge case before any counting or parsing, ensuring getSynonyms always returns a non-null synonyms array as its docblock promises.

What Is Working Well

• NormalizesSynonymGroups trait is focused, composable, and free of duplication. Single responsibility, two methods.
• Divergent behavior between fromApiResponse (throws) and normalizeSynonyms (drops) is documented in fromApiResponse's docblock — a future reader won't be surprised.
• getSynonyms docblock accurately describes the always-non-null synonyms guarantee.
• withSynonymGetDefaults reads as a clean three-field normalization: pick up defaults, guard the array, return.
• Test coverage is thorough across all edge cases raised across five review passes.
• array_merge in testMatchesOpenApiExampleResponse ensures fixture keys override defaults, not the other way around.
• testFromArrayDropsEmptyTermsFromSolrStringGroups directly exercises the fix from this commit.

1. Synonym search type

   • Add synonym to the search-type list (SEARCH_TYPES, method
     definitions/descriptions) so it is selectable in /searchable-fields.
   • Enable it in presets on full-analysis fields only (name, brand, tags;
     not basic-analysis fields like description/categories/sku) across
     Base, PrestaShop, Shopify, and Magento presets.

2. Seed synonyms at index creation (removes save-time downtime)

   • V2IndexManager now compiles the stored synonyms_config into per-language
     SynonymConfiguration objects and passes them in the create-index request,
     so a fresh index carries synonyms and they take effect on activation.
   • Make SynonymPusher::compileGroups() public for reuse; drop the now-orphaned
     pushAll().
   • Remove VersionSwitchForNewIndexJob::rePushSynonyms() — the post-activation
     re-push (close→update→open on the live index) is no longer needed.

@claude review

Claude finished @PauliusInvertus's task in 3m 4s —— View job

Code Review — BRD-804 Synonym Configuration (Sixth Pass)

• Gather context and understand the request
• Read changed files and current diff
• Analyze correctness, edge cases, and test coverage
• Post review feedback

Overview

The latest commit (63bf39d) addresses SteponasK's change-request by adding synonym seeding directly to IndexCreateRequest. The layering is clean: callers now pass SynonymConfiguration[] to the constructor (or via withSynonyms() / withAddedSynonym()), and the request serializes them as Solr strings in the create-index payload — so synonyms are live the moment the index activates.

All prior review feedback (five passes worth) is fully resolved. Three issues remain in the new commit.

Issues

1. withAddedSynonym() has no test

withSynonyms() is covered by testWithSynonymsReturnsNewInstance, but its sibling withAddedSynonym() has no test case at all. Given that every other builder method in this class has at least one test, the gap is inconsistent.

src/V2/ValueObjects/Index/IndexCreateRequest.php line 93

public function withAddedSynonym(SynonymConfiguration $synonym): self
{
    return new self($this->locales, $this->fields, [...$this->synonyms, $synonym]);
}

A minimal test verifying that the new synonym is appended and the original is unchanged covers the contract.

Fix this →

2. Duplicate-language detection is absent from validateSynonyms()

The docblock describes the parameter as "per-language synonym configurations" — implying uniqueness by language — but validateSynonyms() only checks the type of each entry. Two SynonymConfiguration objects for the same language silently pass validation and produce a create-index payload with duplicate language keys:

"synonyms": [
    {"language": "en", "synonyms": ["laptop, notebook"]},
    {"language": "en", "synonyms": ["phone, mobile"]}
]

The API's behaviour in this case is undefined. A simple uniqueness check at validation time:

$languages = [];
foreach ($synonyms as $index => $synonym) {
    if (!$synonym instanceof SynonymConfiguration) { /* existing throw */ }

    if (isset($languages[$synonym->language])) {
        throw new InvalidArgumentException(
            sprintf(
                'Duplicate synonym configuration for language "%s" at index %d.',
                $synonym->language,
                $index
            ),
            'synonyms',
            $synonyms
        );
    }
    $languages[$synonym->language] = true;
}

Fix this →

3. Synonym passthrough in existing with*() methods is untested

All four existing builder methods — withLocales(), withFields(), withAddedLocale(), withAddedField() — were updated to thread $this->synonyms through. None of the new or existing tests verify that synonyms survive these calls. If a future refactor accidentally drops the third constructor argument again, no test will catch it.

A single parameterized test that calls each builder on a request with synonyms and asserts the synonym list is unchanged covers all four:

public function testExistingBuilderMethodsPreserveSynonyms(): void
{
    $synonym = new SynonymConfiguration('en', [['laptop', 'notebook']]);
    $base = new IndexCreateRequest(['en-US'], [new FieldDefinition('id', FieldType::KEYWORD)], [$synonym]);

    $this->assertEquals([$synonym], $base->withLocales(['lt-LT'])->synonyms);
    $this->assertEquals([$synonym], $base->withFields([])->synonyms);
    $this->assertEquals([$synonym], $base->withAddedLocale('lt-LT')->synonyms);
    $this->assertEquals([$synonym], $base->withAddedField(new FieldDefinition('name', FieldType::TEXT))->synonyms);
}

Fix this →

Minor Observation

The June 25 PR description references application-layer changes — V2IndexManager, SynonymPusher::compileGroups(), VersionSwitchForNewIndexJob::rePushSynonyms(), and synonym search-type additions — that don't appear in this SDK's diff. These look like consumer-side changes in the module that uses this SDK, not SDK changes themselves. Worth a confirmation that they are handled separately before merge.

What Is Working Well

• Backward-compatible: $synonyms = [] default means existing callers need no changes.
• jsonSerialize() omits the synonyms key entirely when the array is empty — clean contract.
• withSynonyms() and withAddedSynonym() follow the existing builder pattern exactly.
• Tests cover the default-empty case, Solr-string serialization, immutability via withSynonyms(), and invalid-type rejection — solid coverage for the happy and error paths.
• The NormalizesSynonymGroups trait, SynonymConfiguration::fromApiResponse(), and SynonymResponse changes from prior commits continue to hold up; no regressions introduced.